Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

BREAKING-CHANGE: new ChatMessageContent for style caching #24691

Merged
merged 21 commits into from
Sep 26, 2022
Merged

BREAKING-CHANGE: new ChatMessageContent for style caching #24691

merged 21 commits into from
Sep 26, 2022

Conversation

YuanboXue-Amber
Copy link
Contributor

@YuanboXue-Amber YuanboXue-Amber commented Sep 7, 2022 •
edited
Loading

Create new ChatMessageContent component to fix style caching.

Breaking change:

Styles that are applied on ChatMessage content slot should now be moved to ChatMessageContent root slot

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 7, 2022 •
edited
Loading

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 4b3e3f4:

Sandbox Source
@fluentui/react 8 starter Configuration
@fluentui/react-components 9 starter Configuration

@size-auditor
Copy link

size-auditor bot commented Sep 7, 2022 •
edited
Loading

Asset size changes

Project Bundle Baseline Size New Size Difference
office-ui-fabric-react fluentui-react-northstar-Chat 159.484 kB 160.18 kB ExceedsBaseline     696 bytes

ExceedsTolerance Over Tolerance (1024 B) ExceedsBaseline Over Baseline BelowBaseline Below Baseline New New Deleted  Removed 1 kB = 1000 B

Baseline commit: 9b4a6b41db93754a43b8057a1d1881ecc7bd5802 (build)

@fabricteam
Copy link
Collaborator

fabricteam commented Sep 7, 2022 •
edited
Loading

📊 Bundle size report

🤖 This report was generated against 9b4a6b41db93754a43b8057a1d1881ecc7bd5802

@fabricteam
Copy link
Collaborator

fabricteam commented Sep 7, 2022 •
edited
Loading

Perf Analysis (@fluentui/react-northstar)

⚠️ 6 potential perf regressions detected

Potential regressions comparing to master

Scenario Current PR Ticks Baseline Ticks Ratio Regression Analysis
TreeWith60ListItems.default 145 133 1.09:1 analysis
ButtonMinimalPerf.default 133 127 1.05:1 analysis
AttachmentMinimalPerf.default 115 112 1.03:1 analysis
RefMinimalPerf.default 180 178 1.01:1 analysis
PortalMinimalPerf.default 139 142 0.98:1 analysis
AccordionMinimalPerf.default 113 120 0.94:1 analysis
Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
AvatarMinimalPerf.default 160 148 1.08:1
ListWith60ListItems.default 528 489 1.08:1
FormMinimalPerf.default 315 298 1.06:1
GridMinimalPerf.default 282 269 1.05:1
SegmentMinimalPerf.default 289 276 1.05:1
IconMinimalPerf.default 556 530 1.05:1
CardMinimalPerf.default 443 424 1.04:1
LayoutMinimalPerf.default 302 290 1.04:1
TableMinimalPerf.default 330 316 1.04:1
ToolbarMinimalPerf.default 777 749 1.04:1
CarouselMinimalPerf.default 386 375 1.03:1
StatusMinimalPerf.default 564 547 1.03:1
TextAreaMinimalPerf.default 388 376 1.03:1
AttachmentSlotsPerf.default 914 893 1.02:1
DialogMinimalPerf.default 645 634 1.02:1
FlexMinimalPerf.default 231 227 1.02:1
ListNestedPerf.default 468 458 1.02:1
MenuButtonMinimalPerf.default 1446 1418 1.02:1
RosterPerf.default 1783 1756 1.02:1
ProviderMinimalPerf.default 343 335 1.02:1
RadioGroupMinimalPerf.default 370 361 1.02:1
ButtonSlotsPerf.default 453 448 1.01:1
ChatMinimalPerf.default 592 588 1.01:1
DropdownManyItemsPerf.default 546 542 1.01:1
DropdownMinimalPerf.default 2240 2228 1.01:1
EmbedMinimalPerf.default 3078 3061 1.01:1
HeaderSlotsPerf.default 634 625 1.01:1
InputMinimalPerf.default 955 945 1.01:1
PopupMinimalPerf.default 534 531 1.01:1
ReactionMinimalPerf.default 311 307 1.01:1
SkeletonMinimalPerf.default 279 277 1.01:1
TableManyItemsPerf.default 1556 1536 1.01:1
TooltipMinimalPerf.default 1988 1964 1.01:1
ButtonOverridesMissPerf.default 1084 1085 1:1
CheckboxMinimalPerf.default 1720 1725 1:1
DatepickerMinimalPerf.default 4778 4772 1:1
HeaderMinimalPerf.default 287 286 1:1
ImageMinimalPerf.default 308 307 1:1
LabelMinimalPerf.default 312 311 1:1
ListMinimalPerf.default 427 428 1:1
ProviderMergeThemesPerf.default 1055 1060 1:1
SliderMinimalPerf.default 1337 1332 1:1
SplitButtonMinimalPerf.default 3626 3629 1:1
CustomToolbarPrototype.default 2252 2241 1:1
TreeMinimalPerf.default 666 666 1:1
AnimationMinimalPerf.default 430 436 0.99:1
DividerMinimalPerf.default 285 289 0.99:1
ItemLayoutMinimalPerf.default 975 986 0.99:1
ListCommonPerf.default 516 523 0.99:1
LoaderMinimalPerf.default 559 563 0.99:1
MenuMinimalPerf.default 709 721 0.98:1
TextMinimalPerf.default 268 274 0.98:1
BoxMinimalPerf.default 268 276 0.97:1
AlertMinimalPerf.default 211 220 0.96:1
ChatWithPopoverPerf.default 295 311 0.95:1
VideoMinimalPerf.default 587 619 0.95:1
ChatDuplicateMessagesPerf.default 225 241 0.93:1

@microsoft microsoft deleted a comment from azure-pipelines bot Sep 7, 2022
@YuanboXue-Amber YuanboXue-Amber added the Ready for VR Used to trigger screener checks for PRs label Sep 9, 2022
@YuanboXue-Amber YuanboXue-Amber marked this pull request as ready for review September 9, 2022 16:13
@YuanboXue-Amber YuanboXue-Amber requested review from a team and Hirse as code owners September 9, 2022 16:13
@YuanboXue-Amber YuanboXue-Amber changed the title [DRAFT] chat message style caching BREAKING-CHANGE: new ChatMessageContent for style caching Sep 9, 2022
YuanboXue-Amber and others added 5 commits September 12, 2022 12:10
@YuanboXue-Amber @layershifter
Update packages/fluentui/docs/src/examples/components/Chat/Playground…
adb1c67 
….tsx

Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>
@YuanboXue-Amber @layershifter
Update packages/fluentui/react-northstar/src/themes/teams/components/…
3a0752e 
…Chat/chatMessageContentStyles.ts

Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>
@YuanboXue-Amber
apply comment
4a9d7f3
@YuanboXue-Amber
remove old classname
6db6cfb
@YuanboXue-Amber
remove old classname
d17de44
@Hirse Hirse requested a review from dvdzkwsk September 13, 2022 16:31
Hirse
Hirse reviewed Sep 13, 2022
View reviewed changes
packages/fluentui/react-northstar/src/themes/teams/components/Chat/chatMessageContentStyles.ts Outdated Show resolved Hide resolved
packages/fluentui/react-northstar/src/themes/teams/components/Chat/chatMessageContentStyles.ts Outdated
textDecoration: 'underline',
},
},
...getChatMessageVariantStyles(componentStyleFunctionParam),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have this as a function? I only see it used once.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to keep the other layout-specific styles separated. But now looking at it, removing the function makes more sense.

packages/fluentui/react-northstar/src/themes/teams/components/Chat/chatMessageContentStyles.ts
display: 'block',
'& a': {
outline: 'none',
color: p.mine ? v.linkColorMine : v.linkColor,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davezuko Is this how we're styling links in TMP?
If we don't use it, let's just remove it here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In TMP it looks like links are colored the same for both my/their messages, so this doesn't seem necessary.

packages/fluentui/react-northstar/src/themes/teams/components/Chat/chatMessageContentStyles.ts Outdated
}) => {
return {
...(p.layout === 'refresh' &&
p.density === 'comfy' && {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does the new link design only apply to comfy density?
Shouldn't it be consistent for all refresh designs? (@davezuko)

@YuanboXue-Amber
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@YuanboXue-Amber YuanboXue-Amber merged commit 0e50bf2 into microsoft:master Sep 26, 2022
@YuanboXue-Amber YuanboXue-Amber deleted the chatmsg branch September 26, 2022 12:47
marcosmoura added a commit to marcosmoura/fluentui that referenced this pull request Sep 28, 2022
@marcosmoura
Merge branch 'master' into marcosmoura/feat/selectable-cards
419ff48 
* master: (21 commits)
  chore: Migrate react-avatar to use new build (microsoft#24969)
  applying package updates
  chore(react-input, react-textarea): Deprecating filled with shadow appearance variants (microsoft#24900)
  fix: v8 Dropdown no longer sets incorrect and unnecessary aria-activedescendant (microsoft#24593)
  feat: v0 Tooltip migration from v9 (microsoft#24908)
  chore: bump devDeps to fix critical security vulnerability (microsoft#24891)
  Fixing Tree chart issues (microsoft#24752)
  init: new package react-avatar-context (microsoft#24968)
  ci(.github): add issues write permisions to triage-bot worflow (microsoft#24963)
  applying package updates
  fix(Toolbar): close previous submenu when opening another submenu (microsoft#24836)
  fix: update non-focus-trap Popover role to be group (microsoft#24897)
  feat: Avatar's aria label includes 'active' or 'inactive' when using the active prop (microsoft#24901)
  feat(scripts): implement triage-bot module (microsoft#24911)
  chore: bump @octokit/rest to v18 (microsoft#24919)
  stress test: add "build-fixture" command (microsoft#24928)
  BREAKING-CHANGE: new ChatMessageContent for style caching (microsoft#24691)
  bugfix: fix changefile to properly update version of react-components with a patch (microsoft#24949)
  feat(scripts): enable strict checking for additional sub-folders(packages) (microsoft#24526)
  chore: exports DialogContent as unstable (microsoft#24943)
  ...
NotWoods pushed a commit to NotWoods/fluentui that referenced this pull request Nov 18, 2022
@YuanboXue-Amber @layershifter @NotWoods
BREAKING-CHANGE: new ChatMessageContent for style caching (microsoft#…
1ab91a9 
…24691)

* new chat msg content

* fix playground

* export them

* UT

* remove cruft

* fix style and example

* add missing props

* fix unhandled

* chglog

* chglog

* Update packages/fluentui/docs/src/examples/components/Chat/Playground.tsx

Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>

* Update packages/fluentui/react-northstar/src/themes/teams/components/Chat/chatMessageContentStyles.ts

Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>

* apply comment

* remove old classname

* remove old classname

* small prop update

* Update packages/fluentui/react-northstar/src/themes/teams/components/Chat/chatMessageContentStyles.ts

* remove function in chatmsgcontentstyles

* remove function in chatmsgcontentstyles

Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>
@khmakoto khmakoto added Fluent UI react-northstar (v0) Work related to Fluent UI V0 and removed Fluent UI react-northstar labels Nov 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hirse Hirse Hirse left review comments

@layershifter layershifter layershifter approved these changes

@ling1726 ling1726 ling1726 approved these changes

@dvdzkwsk dvdzkwsk Awaiting requested review from dvdzkwsk

Assignees

@Hotell Hotell

Labels
Fluent UI react-northstar (v0) Work related to Fluent UI V0 Ready for VR Used to trigger screener checks for PRs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@YuanboXue-Amber @fabricteam @Hirse @dvdzkwsk @layershifter @ling1726 @Hotell @khmakoto @msft-fluent-ui-bot